Repository navigation
Tidy the investment graph module - #1576
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1576 +/- ##
==========================================
- Coverage 90.90% 90.89% -0.02%
==========================================
Files 61 61
Lines 9116 9138 +22
Branches 9116 9138 +22
==========================================
+ Hits 8287 8306 +19
+ Misses 507 505 -2
- Partials 322 327 +5 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
🟢 Approval recommended
No active review issues are identified for the investment graph refactor.
No open issues 🎉
What changed in this PR
This PR simplifies the investment graph module, retaining the existing approach to ordering markets and grouping independent investment decisions.
Changes:
- Refactors graph filtering and cycle condensation to use commodity graph indices directly.
- Replaces
order_sccswith a per-componentorder_scchelper and simplifies test setup and assertions. - Updates documentation and a pricing comment to reference the renamed helper.
| File | Description |
|---|---|
src/simulation/prices.rs |
Updates the helper name in a comment; no functional change. |
src/graph/investment.rs |
Simplifies investment graph construction, cycle ordering, and test helpers. |
docs/model/commodity_graphs.md |
Updates the cycle-ordering API reference and link. |
Review effort: Balanced
6cc3e82 to
c0efc7a
Compare
alexdewar
left a comment
alexdewar
left a comment
There was a problem hiding this comment.
LGTM! Always nice to have a PR that removes code.
| /// | ||
| /// Components with one market become `MarketSet::Single`, and those with multiple markets become | ||
| /// `MarketSet::Cycle`. | ||
| fn to_market_set(graph: &CommoditiesGraph, scc: &[NodeIndex]) -> MarketSet { |
There was a problem hiding this comment.
You could define a from_scc method for MarketSet instead. Up to you.
| ) { | ||
| /// `scc` holds the commodity graph indices of the SCC's markets, and is sorted in place into the | ||
| /// final order. If the MILP cannot be solved, a warning is logged and the order is left unchanged. | ||
| fn order_scc(graph: &CommoditiesGraph, commodities: &CommodityMap, scc: &mut [NodeIndex]) { |
There was a problem hiding this comment.
Just noting that there are a lot of Vecs of size n in this function, whereas you could have a smaller number (maybe just one) with a struct containing different fields in. I'm not saying you should do that, just that it's an option. The downside of manually working with indices is that code is more prone to off-by-one errors etc. (using iterators may be more idiomatic). That said, it's pretty clear as it stands (notwithstanding the fact that I don't actually understand the algorithm in any detail 😄).
Description
Tidying up the investment graph module in preparation for #1557 and future work. Heavily aided by AI. My initial attempt at #1557 was a bit clunky, so I asked for ideas to tidy up the module, make these refactoring changes on the base branch, then rebase #1557 on top, therefore separating refactoring from the feature changes. I also told it some ideas about future changes, where I expect we'll switch the investment graph to have agents as nodes rather than market sets (just a vague idea at this point), and to keep this in the back of its mind.
Main changes:
order_sccshas becomeorder_scc- ordering one SCC rather than all of them, called in a loop. I think this makes the function a bit cleaner and removes a nesting level.init_investment_graphentirely. Graphs are now initialised with a singlefilter_mapcall. The current approachfilter_maps, then copies nodes and edges to a new graph, which I think is a leftover from a time where at this point we were combining multiple region-level graphs, but no longer have to do this after Combine region graphs into single graph for each year #1569.MarketSetuntil after cycle detection and ordering. Previously we made the switch before, which meant that we had to pass two different graphs toorder_sccs(one with (commodity, region) nodes and one withMarketSetnodes). This is way cleaner now.is_marketandto_market_setI think this is undoubtedly an improvement.
Fixes # (issue)
Type of change
Key checklist
$ cargo test$ cargo docpresent in the previous release
Further checks